Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

I like this PR in general!

Are there performance / memory implications with these changes? Maybe some benchmarks would be great with the before and after comparison.

Are there performance / memory implications with these changes? Maybe some benchmarks would be great with the before and after comparison.

That's a very difficult question to answer because the "before" case that this PR is targeting would have resulted in a stack overflow, so the test would be "it works at all" vs "Segmentation Fault".

In terms of non-circular references the heaviest memory implication would be temporarily holding on to a second copy of a model's attributes only on the toArray() method, but that would be cleared as soon as the toArray() for a specific model finishes. If there was an initial "working" implementation that didn't cause a segfault, this implementation would use less processing/memory because every time toArray() was called on a model recursively it would return the cached copy instead of re-calculating it again... But again, that's entirely theoretical because that case would never work originally.

My understanding of WeakMap in general won't really use much memory because it only actually holds a weak reference to an object that already exists in memory (so essentially just a pointer), instead of a clone of the object. Effectively the only extra memory that it uses would be the cached value, which is always a Boolean true for push(), and an empty array for getQueueableRelations(). toArray() is the only method that stores a significant payload in the cache, and the finally block should always clear that up, even if an exception was thrown somewhere in the call.

The inverse() PR (#51582) that this PR is a companion to would result in lower memory usage and fewer DB calls in a number of situations.

That's a very difficult question to answer because the "before" case that this PR is targeting would have resulted in a stack overflow, so the test would be "it works at all" vs "Segmentation Fault".

Yeah, I meant regarding a non-circular toArray call, because it also uses the without Recursion part.

Yeah, I meant regarding a non-circular toArray call, because it also uses the without Recursion part.

So there would be extra memory usage due to attributesToArray() being cached in case there's recursion. I could mitigate against it but it would only add extra complexity to address something that is probably not going to cause major issues for the majority of use cases. I'm already not 100% happy with the current increase to complexity for toArray(), but I'm hedging memory usage against processing time in response to @Tofandel 's review. If it's a problem for specific users, they can always swap out toArray() on affected models.

@samlev really dig the PR, but unfortunately it is a pretty big hit on performance. For example, seed 1,000 users into the database and return User::all()->toArray() from a route. That toArray call is 2x slower on this PR compared to current 11.x branch. On my machine, I jump from roughly 40-50ms to about 100ms on this PR.

use App\Models\User;
use Illuminate\Support\Benchmark;

$users = User::all();

Benchmark::dd(fn () => $users->toArray());

@taylorotwell There's definitely some ways to clean it up more. I'm guessing that the speed issue is the additional call to attributesToArray() which could actually be done as an "on demand"/"when needed" - I was just trying to avoid adding even more complexity.

@samlev we may just have to give the extra complexity a shot - hopefully won't be too bad. I don't think we can stomach a 2x performance hit for all Eloquent serialization.

@taylorotwell this new update should negate the performance penalty for non-circular relations (and should more or less resolve @Jubeki 's concerns about increased memory usage).

The only other "improvement" that I can think of would be to get extremely meta by forcing toArray() to be called recursively:

public function toArray()
{
    return $this->withoutRecursion(
        fn () => array_merge($this->toArray(), $this->relationsToArray()),
        fn () => $this->attributesToArray(),
    );
}

which would have the effect of only calling attributesToArray() once to fulfill the second toArray() call and give the default value. So the call stack would go something like this (visualising recursive call stacks in text is hard...):

toArray() {
  withoutRecursion() {
    toArray() {
      withoutRecursion() {
        attributesToArray();
      }
    }
    relationsToArray();
  }
}

The end result, though, is that attributesToArray() would be called only once, and cached, then merged with relationsToArray() which would (ultimately) use the cached copy of attributesToArray() if it does actually have circular references.

@samlev Indeed I don't see the performance regression anymore. Nice! Will do a final review in the morning and try to get this merged. 👍

Good to see this being handled, but isn't using a flag a better approach? Regarding performance?

Something like this:

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\Relations\HasMany;

class Item extends Model
{
    // recursion flag
    protected $visited = false;

    public function item(): BelongsTo
    {
        return $this->belongsTo(Item::class);
    }

    public function items(): HasMany
    {
        return $this->hasMany(Item::class);
    }

    public function toArray(): array
    {
        if ($this->visited) {
            // TODO: improve the fallback case, maybe an array 
            // with a single marker value to filter out later?
            return [];
        }

        $this->visited = true;

        try {
            return parent::toArray();
        } finally {
            $this->visited = false;
        }
    }
}

With this test code:

<?php

use App\Models\Item;
use Illuminate\Support\Facades\Artisan;

Artisan::command('circular', function () {
    $parent = Item::query()->create();
    $parent->items()->create();
    $parent->items()->create();
    $parent->items()->create();

    $parent->load(['items']);

    // set parent instance manually to create the circular reference
    $parent->items->each->setRelation('item', $parent);

    $this->info($parent->toJson(\JSON_PRETTY_PRINT));

    // print twice to show the visited property is cleared nicely
    $this->info($parent->toJson(\JSON_PRETTY_PRINT));
});

Yields this output:

$ php artisan circular
{
    "updated_at": "2024-08-22T05:28:24.000000Z",
    "created_at": "2024-08-22T05:28:24.000000Z",
    "id": 1,
    "items": [
        {
            "id": 2,
            "item_id": 1,
            "created_at": "2024-08-22T05:28:24.000000Z",
            "updated_at": "2024-08-22T05:28:24.000000Z",
            "item": []
        },
        {
            "id": 3,
            "item_id": 1,
            "created_at": "2024-08-22T05:28:24.000000Z",
            "updated_at": "2024-08-22T05:28:24.000000Z",
            "item": []
        },
        {
            "id": 4,
            "item_id": 1,
            "created_at": "2024-08-22T05:28:24.000000Z",
            "updated_at": "2024-08-22T05:28:24.000000Z",
            "item": []
        }
    ]
}
{
    "updated_at": "2024-08-22T05:28:24.000000Z",
    "created_at": "2024-08-22T05:28:24.000000Z",
    "id": 1,
    "items": [
        {
            "id": 2,
            "item_id": 1,
            "created_at": "2024-08-22T05:28:24.000000Z",
            "updated_at": "2024-08-22T05:28:24.000000Z",
            "item": []
        },
        {
            "id": 3,
            "item_id": 1,
            "created_at": "2024-08-22T05:28:24.000000Z",
            "updated_at": "2024-08-22T05:28:24.000000Z",
            "item": []
        },
        {
            "id": 4,
            "item_id": 1,
            "created_at": "2024-08-22T05:28:24.000000Z",
            "updated_at": "2024-08-22T05:28:24.000000Z",
            "item": []
        }
    ]
}

Of course, there is this dangling array on the child elements' parent references, but this is just a proof of concept.

I thought on hacking with the Model@relationsToArray() method to filter them out, but didn't want to invest time on it before asking for feedback.

EDIT: without the overridden Item@toArray() method, the command above segfaults.

Yes, adding a marker will remove the dangling ones:

<?php

namespace App;

enum Visited
{
    case VISITED;
}

Changed model to account for the marker:

<?php

namespace App\Models;

use App\Visited;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\Relations\HasMany;

class Item extends Model
{
    // recursion flag
    protected $visited = false;

    public function item(): BelongsTo
    {
        return $this->belongsTo(Item::class);
    }

    public function items(): HasMany
    {
        return $this->hasMany(Item::class);
    }

    public function toArray(): array
    {
        if ($this->visited) {
            return [Visited::VISITED];
        }

        $this->visited = true;

        try {
            return parent::toArray();
        } finally {
            $this->visited = false;
        }
    }

    public function relationsToArray(): array
    {
        // filters out visited relations
        return \array_filter(parent::relationsToArray(), fn ($item) => $item !== [Visited::VISITED]);
    }
}

Updated output (with a single toJson() call):

$ php artisan circular
{
    "updated_at": "2024-08-22T05:43:34.000000Z",
    "created_at": "2024-08-22T05:43:34.000000Z",
    "id": 1,
    "items": [
        {
            "id": 2,
            "item_id": 1,
            "created_at": "2024-08-22T05:43:34.000000Z",
            "updated_at": "2024-08-22T05:43:34.000000Z"
        },
        {
            "id": 3,
            "item_id": 1,
            "created_at": "2024-08-22T05:43:34.000000Z",
            "updated_at": "2024-08-22T05:43:34.000000Z"
        },
        {
            "id": 4,
            "item_id": 1,
            "created_at": "2024-08-22T05:43:35.000000Z",
            "updated_at": "2024-08-22T05:43:35.000000Z"
        }
    ]
}

Of course, instead of overriding those methods on user land, we can consider incorporating them upstream.

Just for easiness to reproduce (e.g. copy and paste), here is the migration I used:

<?php

use App\Models\Item;
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
    public function up(): void
    {
        Schema::create('items', function (Blueprint $table) {
            $table->id();
            $table->foreignIdFor(Item::class)->nullable()->constrained();
            $table->timestamps();
        });
    }

    public function down(): void
    {
        Schema::dropIfExists('items');
    }
};

Reproducible repo: https://github.com/rodrigopedra/circular

P.S.: moved overrides to a trait

@rodrigopedra the minor difference between the two implementations here is that the nested children will still have the relation, they just won't have the relation of the relations.

So with your example (timestamps removed for brevity):

{
    "id": 1,
    "items": [
        {
            "id": 2,
            "item_id": 1
        },
        {
            "id": 3,
            "item_id": 1
        },
        {
            "id": 4,
            "item_id": 1
        }
    ]
}

The implementation in this PR would still have the "item" on each of the children:

{
    "id": 1,
    "items": [
        {
            "id": 2,
            "item_id": 1,
            "item": {
                "id": 1
            }
        },
        {
            "id": 3,
            "item_id": 1,
            "item": {
                "id": 1
            }
        },
        {
            "id": 4,
            "item_id": 1,
            "item": {
                "id": 1
            }
        }
    ]
}

This doesn't seem super important, but it's more "accurate" as to the actual state of each object, and more consistent with the result if you had eager-loaded the relations (e.g. Item::with('items.item')).

There's more to it, but we also have the potential for a method that causes recursion to call another method that might cause recursion, and we don't want to prevent that second call from happening but we still want to be sure that everything is cleaned up after any individual call to toArray() or push() or getQueueableRelations(). Adding it in this way instead of an individual flag opens up the API to userland to do things which may have otherwise been considered "unsafe" - it's a protected method that people could use themselves in their models when doing something like custom eloquent attributes, etc.

Doing it with a static weak map also opens up possibilities with inspecting levels of recursion (e.g. static::getRecursionCache()->count() would tell you how many individual objects of the current class are currently in play in the current call stack - something that might be useful for diagnostics or observability, or even just limiting the size of the call stack). WeakMaps "clean up" after themselves - or more to the point they don't prevent objects from being garbage collected - so we also don't really have a risk of orphaned records, or holding on to extra memory for any longer than we need to.

Ultimately, this PR isn't just about toArray() - it's resolving an issue that I've personally encounterd a few times, and other people have too, often in situations where they didn't expect to have a "gotcha".

@rodrigopedra yours has an edge case when the relation collection is shared while current PR does not:

$sharedItem = new Item(['name' => 'shared']);

$itemA = new Item(['name' => 'a']);
$itemA->setRelation('shared', $sharedItem);
$itemB = new Item(['name' => 'b']);
$itemB->setRelation('shared', $sharedItem);

$items = Collection::make([$itemA, $itemB]);

$parent = new Item(['name' => 'parent']);
$parent->setRelation('items', $items);

$sharedItem->setRelation('items', $items);

dd($parent->toArray());

Output (dd because you cannot json serialize non-backed enum and it crashes):

array:2 [
  "name" => "parent"
  "items" => array:2 [
    0 => array:2 [
      "name" => "a"
      "shared" => array:2 [
        "name" => "shared"
        "items" => array:2 [
          0 => array:1 [
            0 => App\Visited {#1671
              +name: "VISITED"
            }
          ]
          1 => array:1 [
            "name" => "b"
          ]
        ]
      ]
    ]
    1 => array:2 [
      "name" => "b"
      "shared" => array:2 [
        "name" => "shared"
        "items" => array:2 [
          0 => array:1 [
            "name" => "a"
          ]
          1 => array:1 [
            0 => App\Visited {#1671}
          ]
        ]
      ]
    ]
  ]
]

@samlev thanks, I noticed that when looking at the tests. And you are right, that'd be more accurate.

@donnysim while digging into the PR's code, I moved away from using this marker. But, thank you so much for reviewing it, and for the insights.

@samlev I updated my test repo, and also created a fork to use the same test set as yours (Minus the test cases that, I commented out that I don't quite get the assertion being 0):

11.x...rodrigopedra:framework:11.x

I didn't want to push a new PR, as I don't want to overtake this one.

I understood from your test cases that my alternative implementation was missing the mixed function recursion. So I incorporated the stack trace you have in yours.

Can you review it? I believe it is a simpler approach, and avoids a static global WeakMap.

Nonetheless, thank you very much for this PR, I had to deal with manually unsetting relations on many projects before.

@donnysim, on my POC repository, I added a second command to account for the use case you mentioned.

Follow its README and run php artisan shared

Thanks again for the insights.

The big problem with the flag is if you call the method multiple times after having changed attributes (or even without) for example, then it will not output the correct array, which in theory the withoutRecursion doesn't have

It's a bit of a functional programming vs OOP

@Tofandel as an internal/protected property, that would only happen if someone overrides it manually in an unintended way.

Yet, the same can happen with the static cache. One could just assign a new WeakMap instance to the cache and blow things up.

When I saw the tests I moved away to a single flag, due to the mixed recursive method call, as a single flag prevented that use case.

(check the DatabaseConcernsPreventsCircularRecursionTest and look for the PreventsCircularRecursionWithRecursiveMethod@callOtherStack method)

But I'd still go for a per-instance stack than a global WeakMap. That is just as a matter of preference, the current state of this PR is a great addition, IMO.

EDIT: on the first iteration of the flag-based alternative, the sample command calls the recursive method twice, without any issues.

I was talking about your original implementation in comments, your new implementation is almost the same as the one proposed in this PR and uses a local Map of the callstack as well which doesn't have this issue

I do prefer when things are not static as well (because static always causes unexpected issues when callbacks are involved eg #51825), so if your solution manages all the test cases of this PR and has no additional performance issue, then it's better

@Tofandel it has a different approach for not caching the first ever $default value.

On this PR's implementation, the first ever $default value is cached for future reference, on mine it uses the then current value from the next recursive call.

But nonetheless, apart from the cross recursive calls between different functions, which clearly don't work with the single flag approach, I wonder how keeping a map of call stack hashes is much different than the flag approach, regarding your functional vs OOP comment.

I moved away from the single flag implementation to accommodate the cross recursive calls for different functions. Although I think that for the serialization use case it would be totally fine, as none of the currently affected methods reference each other.

It is ok if you don't want to stretch this further, and thanks again for reviewing my code =)

Thanks @samlev - really well done PR.